Enhance tests #226

Harry-Chen · 2024-10-21T15:13:15Z

Modifications include:

register all tests with ctest framework, so we do not need to call test binaries manually
merge linux and macos CI config files, use strategy matrix to eliminate redundancy
use built-in toolchains of runner image instead installing new ones (they do have some issues -- see actions history in my forked repo for details)
allow CI to be triggered manually (workflow_dispatch)

New CI runs in my fork:

linux: https://github.com/Harry-Chen/reflect-cpp/actions/runs/11443093290
macos-clang: https://github.com/Harry-Chen/reflect-cpp/actions/runs/11442655741
windows-msvc (unmodified): https://github.com/Harry-Chen/reflect-cpp/actions/runs/11442117270

liuzicheng1987 · 2024-10-21T19:54:33Z

@Harry-Chen , thanks for the initiative! Seems like a great PR!

Do you have any idea why this failed for GCC 12? We used to test on GCC 11 and it always worked fine. Also, it seems to have failed on your account as well...

liuzicheng1987 · 2024-10-21T19:56:38Z

@Harry-Chen , obviously the failure didn't even happen during the tests, it happened during the benchmarks, so it is likely that it is due to a bug in one of the libraries we use for benchmarking, not our own library.

Could you just try changing this to GCC 11?

Harry-Chen · 2024-10-22T05:05:17Z

Could you just try changing this to GCC 11?

I can reproduce the segmentation fault locally with GCC 12. I'd prefer trying to fix it first, and treat changing back to GCC 11 as the last resort.

Harry-Chen · 2024-10-22T05:17:17Z

This SIGSEGV goes away when compiling with address sanitizer.

Valgrind says:

==3542836== Use of uninitialised value of size 8
==3542836==    at 0x17EDC1: move_to<rfl::Literal<rfl::internal::StringLiteral<18>{std::array<char, 18>{"FeatureCollection"}}>, rfl::Literal<rfl::internal::StringLiteral<18>{std::array<char, 18>{"FeatureCollection"}}> > (ViewReader.hpp:128)
==3542836==    by 0x17EDC1: assign_if_field_matches<0, rfl::json::Reader::YYJSONInputVar, rfl::NamedTuple<rfl::Field<rfl::internal::StringLiteral<5>{std::array<char, 5>{"type"}}, rfl::Literal<rfl::internal::StringLiteral<18>{std::array<char, 18>{"FeatureCollection"}}>*>, rfl::Field<rfl::internal::StringLiteral<9>{std::array<char, 9>{"features"}}, std::vector<canada_read::Feature, std::allocator<canada_read::Feature> >*> >, std::vector<rfl::Error>, std::array<bool, 2>, std::array<bool, 2> > (ViewReader.hpp:61)
==3542836==    by 0x17EDC1: assign_to_matching_field<0, 1, rfl::json::Reader::YYJSONInputVar, rfl::NamedTuple<rfl::Field<rfl::internal::StringLiteral<5>{std::array<char, 5>{"type"}}, rfl::Literal<rfl::internal::StringLiteral<18>{std::array<char, 18>{"FeatureCollection"}}>*>, rfl::Field<rfl::internal::StringLiteral<9>{std::array<char, 9>{"features"}}, std::vector<canada_read::Feature, std::allocator<canada_read::Feature> >*> >, std::vector<rfl::Error>, std::array<bool, 2>, std::array<bool, 2> > (ViewReader.hpp:102)
==3542836==    by 0x17EDC1: read (ViewReader.hpp:33)

... and ...

==3542836== Invalid write of size 1
==3542836==    at 0x17EDC1: move_to<rfl::Literal<rfl::internal::StringLiteral<18>{std::array<char, 18>{"FeatureCollection"}}>, rfl::Literal<rfl::internal::StringLiteral<18>{std::array<char, 18>{"FeatureCollection"}}> > (ViewReader.hpp:128)
==3542836==    by 0x17EDC1: assign_if_field_matches<0, rfl::json::Reader::YYJSONInputVar, rfl::NamedTuple<rfl::Field<rfl::internal::StringLiteral<5>{std::array<char, 5>{"type"}}, rfl::Literal<rfl::internal::StringLiteral<18>{std::array<char, 18>{"FeatureCollection"}}>*>, rfl::Field<rfl::internal::StringLiteral<9>{std::array<char, 9>{"features"}}, std::vector<canada_read::Feature, std::allocator<canada_read::Feature> >*> >, std::vector<rfl::Error>, std::array<bool, 2>, std::array<bool, 2> > (ViewReader.hpp:61)
==3542836==    by 0x17EDC1: assign_to_matching_field<0, 1, rfl::json::Reader::YYJSONInputVar, rfl::NamedTuple<rfl::Field<rfl::internal::StringLiteral<5>{std::array<char, 5>{"type"}}, rfl::Literal<rfl::internal::StringLiteral<18>{std::array<char, 18>{"FeatureCollection"}}>*>, rfl::Field<rfl::internal::StringLiteral<9>{std::array<char, 9>{"features"}}, std::vector<canada_read::Feature, std::allocator<canada_read::Feature> >*> >, std::vector<rfl::Error>, std::array<bool, 2>, std::array<bool, 2> > (ViewReader.hpp:102)
==3542836==    by 0x17EDC1: read (ViewReader.hpp:33)

So there is a hidden illegal write that should be fixed. It just happens not to crash with other compilers, I believe.

Harry-Chen · 2024-10-22T06:35:33Z

@liuzicheng1987 I do not think it is a problem with 3rd party libraries, but within ViewReader.

GDB says the last frame is:

move_to<rfl::Literal<rfl::internal::StringLiteral<18>{std::array<char, 18>{"FeatureCollection"}}>, rfl::Literal<rfl::internal::StringLiteral<18>{std::array<char, 18>{"FeatureCollection"}}> > (_s=0x7fffffffa658, _t=0x7b7b210000000000)

It runs placement new: ::new (_t) Target(std::move(*_s));. Since _t is obviously a valid pointer, it fails.

(gdb) p *_view
$1 = {static seq_ = {<No data fields>}, values_ = {static size_ = 2, static positions_ = <same as static member of an already seen type>, static seq_ = <same as static member of an already seen type>, static num_bytes_ = 16, data_ = {
      _M_elems = "\000\000\000\000\000!{{DY\202\367\377\177\000"}}}
(gdb) ptype _view
type = class rfl::NamedTuple<rfl::Field<rfl::internal::StringLiteral<5>{std::array<char, 5>{"type"}}, rfl::Literal<rfl::internal::StringLiteral<18>{std::array<char, 18>{"FeatureCollection"}}>*>, rfl::Field<rfl::internal::StringLiteral<9>{std::array<char, 9>{"features"}}, std::vector<canada_read::Feature, std::allocator<canada_read::Feature> >*> >

(gdb) p _view->values_
$4 = {static size_ = 2, static positions_ = {_M_elems = {0, 8, 16}}, static seq_ = <same as static member of an already seen type>, static num_bytes_ = 16, data_ = {_M_elems = "\000\000\000\000\000!{{DY\202\367\377\177\000"}}
(gdb) ptype _view->values_
type = class rfl::Tuple<rfl::Literal<rfl::internal::StringLiteral<18>{std::array<char, 18>{"FeatureCollection"}}>*, std::vector<canada_read::Feature, std::allocator<canada_read::Feature> >*>

So t=0x7b7b210000000000 comes from _view->values_->data[0:8] ("\000\000\000\000\000\21\7b\7b") cast as uint64_t.

After tracing down, the _view used comes from:

reflect-cpp/include/rfl/parsing/Parser_default.hpp

Lines 256 to 270 in b0e4bb7

    
           static Result<T> read_struct(const R& _r, const InputVarType& _var) { 
        
             alignas(T) unsigned char buf[sizeof(T)]; 
        
             auto ptr = std::launder(reinterpret_cast<T*>(buf)); 
        
             auto view = ProcessorsType::template process<T>(to_view(*ptr)); 
        
             using ViewType = std::remove_cvref_t<decltype(view)>; 
        
             const auto [set, err] = 
        
                 Parser<R, W, ViewType, ProcessorsType>::read_view(_r, _var, &view); 
        
             if (err) [[unlikely]] { 
        
               call_destructors_where_necessary(set, &view); 
        
               return *err; 
        
             } 
        
             auto res = Result<T>(std::move(*ptr)); 
        
             call_destructors_where_necessary(set, &view); 
        
             return res; 
        
           }

Along with Valgrind output, we can see the execution chain:

_view is originally a static buffer on stack (with random content).
it is converted to a NamedTuple, whose values_ contains variant type of data.
the first 8 bytes of values_ is converted to a pointer and passed to move_to (thus Use of uninitialised value of size 8).
move_to writes to this random pointer (thus Invalid write of size 1).

I do not currently have the ability to dig further, since I am not familiar with the complex templates used by reflect-cpp.
But I think this information is enough for any core developer to craft a fix.

liuzicheng1987 · 2024-10-22T06:39:35Z

Thanks so much, I will take a look.

Harry-Chen · 2024-10-22T06:42:43Z

reflect-cpp-all-format-benchmarks.gz

And here's a statically-linked binary that could 100% reproduce the issue, should you need it.

liuzicheng1987 · 2024-10-22T20:59:08Z

@Harry-Chen, thanks for bringing this to my attention. The problem is quite a bit deeper and more complex than originally thought, but I will write a fix as quickly as I can. Please give me a couple of days, it requires some deeper changes.

liuzicheng1987 · 2024-10-22T21:00:23Z

@Harry-Chen, but I will certainly merge your PR once I am done. I think your PR is great! Thank you so much!

Harry-Chen · 2024-10-23T03:44:36Z

@Harry-Chen, thanks for bringing this to my attention. The problem is quite a bit deeper and more complex than originally thought, but I will write a fix as quickly as I can. Please give me a couple of days, it requires some deeper changes.

Sure, take your time. Writing correct C++ is never easy 🤦

liuzicheng1987 · 2024-10-29T19:31:18Z

@Harry-Chen , the problem should be resolved with 29c4201.

Could you merge the main branch and add a separate test for GCC 11? The tests should run through for that one, and then I would be too happy to merge your PR.

…ry time Signed-off-by: Shengqi Chen <[email protected]>

Signed-off-by: Shengqi Chen <[email protected]>

THe newly-installed versions have some troublesome issues. Signed-off-by: Shengqi Chen <[email protected]>

Signed-off-by: Shengqi Chen <[email protected]>

… and mac Signed-off-by: Shengqi Chen <[email protected]>

Harry-Chen · 2024-10-30T14:25:01Z

@liuzicheng1987 I added gcc-11 as requested. Additionally I added ccache to speed up compilation when possible.

liuzicheng1987 · 2024-10-30T20:38:35Z

Great work! Thank you so much!

Harry-Chen added 17 commits October 30, 2024 12:28

tests: link to GTest::gtest_main instead of compiling from source eve…

afa6ea6

…ry time Signed-off-by: Shengqi Chen <[email protected]>

tests: register all test binaries to ctest framework

04e9e3f

Signed-off-by: Shengqi Chen <[email protected]>

ci: invoke tests with ctest command

d46b6f0

Signed-off-by: Shengqi Chen <[email protected]>

ci: merge linux-clang configs with job matrix

a8c1e64

Signed-off-by: Shengqi Chen <[email protected]>

ci: merge macos-clang configs with job matrix

3ce55c0

Signed-off-by: Shengqi Chen <[email protected]>

ci: bump versions of actions/checkout

c2300c5

Signed-off-by: Shengqi Chen <[email protected]>

ci: fix syntax in macos-clang

fbf3f9c

Signed-off-by: Shengqi Chen <[email protected]>

ci: further merge linux-{clang,gcc}

76322cf

Signed-off-by: Shengqi Chen <[email protected]>

ci: bump version of KyleMayes/install-llvm-action

bad2d07

Signed-off-by: Shengqi Chen <[email protected]>

ci: fix ninja version detection problem in macos-clang.yaml

c83eb16

Signed-off-by: Shengqi Chen <[email protected]>

ci: disable fail-fast, run linux tests on ubuntu 24.04

577df62

Signed-off-by: Shengqi Chen <[email protected]>

ci: use built-in toolchain from ubuntu-24.04 runner image

7bd494e

THe newly-installed versions have some troublesome issues. Signed-off-by: Shengqi Chen <[email protected]>

ci: disable gh-pages actions in forks to avoid annoying errors

87a58c3

Signed-off-by: Shengqi Chen <[email protected]>

ci: add GCC 11 in Linux test

6f8df39

Signed-off-by: Shengqi Chen <[email protected]>

ci: use customized job name in linux.yaml

5bde679

Signed-off-by: Shengqi Chen <[email protected]>

ci: use ccache to speedup build

7541f8a

Signed-off-by: Shengqi Chen <[email protected]>

ci: disable ccache on windows, use symlinks to invoke ccache on linux…

61eadde

… and mac Signed-off-by: Shengqi Chen <[email protected]>

Harry-Chen force-pushed the enhance-tests branch from 174c71d to 61eadde Compare October 30, 2024 14:23

BestITUserEUW mentioned this pull request Oct 30, 2024

gcc segfault when optimized #240

Closed

liuzicheng1987 merged commit 7bf71c6 into getml:main Oct 30, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance tests #226

Enhance tests #226

Harry-Chen commented Oct 21, 2024

liuzicheng1987 commented Oct 21, 2024

liuzicheng1987 commented Oct 21, 2024

Harry-Chen commented Oct 22, 2024

Harry-Chen commented Oct 22, 2024

Harry-Chen commented Oct 22, 2024

liuzicheng1987 commented Oct 22, 2024

Harry-Chen commented Oct 22, 2024

liuzicheng1987 commented Oct 22, 2024

liuzicheng1987 commented Oct 22, 2024

Harry-Chen commented Oct 23, 2024

liuzicheng1987 commented Oct 29, 2024

Harry-Chen commented Oct 30, 2024

liuzicheng1987 commented Oct 30, 2024

Enhance tests #226

Enhance tests #226

Conversation

Harry-Chen commented Oct 21, 2024

liuzicheng1987 commented Oct 21, 2024

liuzicheng1987 commented Oct 21, 2024

Harry-Chen commented Oct 22, 2024

Harry-Chen commented Oct 22, 2024

Harry-Chen commented Oct 22, 2024

liuzicheng1987 commented Oct 22, 2024

Harry-Chen commented Oct 22, 2024

liuzicheng1987 commented Oct 22, 2024

liuzicheng1987 commented Oct 22, 2024

Harry-Chen commented Oct 23, 2024

liuzicheng1987 commented Oct 29, 2024

Harry-Chen commented Oct 30, 2024

liuzicheng1987 commented Oct 30, 2024